BaseId: warn() instead of raise for duplicate prefixes #73
Conversation
github-actions
bot
added
part:docs
Marenz
requested review from
llucax
and removed request for
florian-wagner-frequenz
src/frequenz/core/id.py
Outdated
| raise TypeError("BaseId cannot be instantiated directly. Use a subclass.") | ||
| # We want to raise an exception here, but currently can't due to | ||
| # https://github.com/frequenz-floss/frequenz-repo-config-python/issues/421 | ||
| warn( | ||
| "BaseId cannot be instantiated directly. Use a subclass.", stacklevel=2 | ||
| ) |
There was a problem hiding this comment.
Wait! This is the wrong exception! It should be this one!
There was a problem hiding this comment.
oh lol. and I guess we have no test for this one 😆
There was a problem hiding this comment.
I guess we don't 🤷
Not sure if it is worth adding now (pytest have some utils to test warnings are emitted).
There was a problem hiding this comment.
You could also add a test that BaseId raises if instantiated directly, this one should be easy too with with pytest.raises(TypeError, match="..."): BaseId(1))
Signed-off-by: Mathias L. Baumann <[email protected]>
Signed-off-by: Mathias L. Baumann <[email protected]>
github-actions
bot
added
the
part:tests
There was a problem hiding this comment.
Pull Request Overview
This PR modifies the behavior of duplicate prefix handling in BaseId to issue a warning rather than immediately raising an exception. Key changes include:
- Adding tests for direct BaseId instantiation and non-unique prefix warning.
- Updating BaseId to use warn() for duplicate prefix detection.
- Adjusting release notes to reflect the change in behavior.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/test_id.py | Added tests to validate subclass instantiation and warning for duplicate prefixes. |
| src/frequenz/core/id.py | Changed duplicate prefix handling from raising ValueError to using warn(). |
| RELEASE_NOTES.md | Updated the release notes to summarize the change in duplicate prefix behavior. |
Comments suppressed due to low confidence (1)
src/frequenz/core/id.py:108
- The docstring for init_subclass still indicates that a ValueError is raised for duplicate prefixes, but the code now issues a warning. Consider updating the docstring to reflect the current behavior.
Raises:
| warn( | ||
| f"Prefix '{str_prefix}' is already registered. " | ||
| "ID prefixes must be unique." | ||
| "ID prefixes must be unique.", |
There was a problem hiding this comment.
It is recommended to explicitly specify the warning category (e.g., UserWarning) in the warn() call for clarity and consistency, even though the default is UserWarning.
| "ID prefixes must be unique.", | |
| "ID prefixes must be unique.", | |
| category=UserWarning, |
2 participants
This is only necessary because to cope with frequenz-floss/frequenz-repo-config-python#421. The check can raise an exception again once that it fixed.